New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kv: fix some bugs in memdb #10550
kv: fix some bugs in memdb #10550
Conversation
db00156
to
71f364f
Compare
size_t pos = m_key_value.first.find(KEY_DELIM, 0); | ||
if (pos == std::string::npos) { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, why is there a key that doesn't have KEY_DELIM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After checking for iterator invalidation, I no longer hit the 'assert_fail'. So I will delete this commit.
rebase please? |
71f364f
to
8c3365c
Compare
@liewegas have updated and rebased. |
Ok, I think in order to fix this properly, we need a more complicated solution. We can't have any update to the kv db anywhere invalidate all iterators. And a sequence like (1) create iterator, (2) insert value and invalidate, (3) create second iterator, (4) use first iterator leads to the same problem (using the invalid iterator created in (1)). Instead,
|
@@ -397,7 +401,7 @@ void MemDB::MDBWholeSpaceIteratorImpl::fill_current() | |||
|
|||
bool MemDB::MDBWholeSpaceIteratorImpl::valid() | |||
{ | |||
if (m_key_value.first.empty()) { | |||
if (!(*is_valid) || m_key_value.first.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest looks fine, But I am not sure if current iterator becomes invalid then we should return invalid or restart iterator from current position. And if we return invalid, will bluestore take care of this and restart new iterator. I will confirm soon,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not notice @liewegas comment . His comment supersedes mine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After keeping track of seqno, I think just restarting the iterator with m_key_value.first as key should work. m_iter = m_btree_p->lower_bound(k);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chhabaramesh @liewegas Thanks for your comments.
I'm not sure if bluestore will retry to get a valid iterator before, so I just add a bool. Then I will fix it with seqno:)
8c3365c
to
b8551bd
Compare
|
||
public: | ||
MDBWholeSpaceIteratorImpl(btree::btree_map<std::string, bufferptr> *btree_p, | ||
std::mutex *btree_lock_p) { | ||
std::mutex *btree_lock_p, uint64_t *btree_seq_) { | ||
m_btree_p = btree_p; | ||
m_btree_lock_p = btree_lock_p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to take btree lock here.
Also please fix indentation.
@tanghaodong25 I have some minor comments on latest code. Please fix those and retest. Overall looks fine to me. |
b8551bd
to
26aaa7c
Compare
@chhabaramesh Thanks. :) |
@@ -472,24 +494,24 @@ int MemDB::MDBWholeSpaceIteratorImpl::seek_to_first(const std::string &k) | |||
{ | |||
std::lock_guard<std::mutex> l(*m_btree_lock_p); | |||
free_last(); | |||
if (k.empty()) { | |||
if (k == "NULL") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure k.empty() is the right thing here...
26aaa7c
to
17cdc2d
Compare
@liewegas updated. Thanks. |
if (m_iter == m_btree_p->end()) { | ||
return -1; | ||
} | ||
fill_current(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two changes are fixing an unrelated bug in seek_to_*, right? Can you put them in a separate commit?
It would be great to have a test for this in test/objectstore/test_kv.cc... :) |
@tanghaodong25, please also run ceph_test_objectstore. |
Using memdb for bluestore kvbackend, we will hit segfault when we use 'kill' command to shut down osd process. After destructing pg, some reference to bluestore will be release, but bluestore has been deleted at this time. Signed-off-by: Haodong Tang <haodong.tang@intel.com>
Signed-off-by: Haodong Tang <haodong.tang@intel.com>
Signed-off-by: Haodong Tang <haodong.tang@intel.com>
Signed-off-by: Haodong Tang <haodong.tang@intel.com>
17cdc2d
to
f5a320c
Compare
@chhabaramesh @liewegas All test in "ceph_test_objectstore" and "ceph_test_keyvaluedb" passed. :) |
I'm trying to replace rocksdb with memdb to identify the overheads of rocksdb for bluestore, but I hit a few errors.
Signed-off-by: Haodong Tang haodong.tang@intel.com